fix: handle deprecating relation for polyrelation#27
Merged
brandonc merged 2 commits intohashicorp:mainfrom Jan 7, 2025
Merged
Conversation
brandonc
previously approved these changes
Jan 7, 2025
Comment on lines
+501
to
+504
| if pFieldType, ok := polyrelationFields[args[1]]; ok && fieldValue.Type() != pFieldType { | ||
| continue | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
I think the solution makes sense but is not at all obvious to the casual reader. Would you mind adding a comment about what this achieves?
Suggested change
| if pFieldType, ok := polyrelationFields[args[1]]; ok && fieldValue.Type() != pFieldType { | |
| continue | |
| } | |
| // Skipping nonmatching types when a polymorphic relation of this name exists enables | |
| // a migration path from relation to polyrelation. | |
| if pFieldType, ok := polyrelationFields[args[1]]; ok && fieldValue.Type() != pFieldType { | |
| continue | |
| } | |
brandonc
approved these changes
Jan 7, 2025
mohanmanikanta2299
approved these changes
Jan 7, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The purpose of this PR is to support deprecating struct fields marked with
relationtags in favor ofpolyrelation. For example, this struct fails to unmarshal in it's current form:This will work if the fruit struct is only ever of the following form:
This is what we have in go-tfe in a handful of places. For example https://github.com/hashicorp/go-tfe/blob/98975f447700482a79e4d7d177cd406ff8132f1d/run_trigger.go#L59-L61
Most of the usages in go-tfe support
polyrelationwith a deprecatedrelationtag, but only one type in the associated polyrelation struct. See https://github.com/hashicorp/go-tfe/blob/98975f447700482a79e4d7d177cd406ff8132f1d/run_trigger.go#L59-L61The issue is that as soon as you try to add another type to the polyrelation/choice struct, we start to run into unmarshal failures. This can be seen in the error handling workaround for
DataRetentionPolicyin go-tfe Organization and Workspace: https://github.com/hashicorp/go-tfe/blob/98975f447700482a79e4d7d177cd406ff8132f1d/organization.go#L500-L505Essentially what's happening here is that we aren't actually allowing a deprecated field to be adopted over time, but forces the library user to immediately adopt the breaking change.
This is a prerequisite to support hashicorp/go-tfe#1016